Update phased trigger and digitizer #799

ryankrebs016 · 2025-01-20T16:37:40Z

Changes:

Refactor phased array module so it acts as a base to the phased array triggers to reduce identical code between modules.
Added digital envelope trigger that is an optional trigger for RNO-G.
Remove extra -1 when calculating V/ADC during digitization.
Small things to accommodate the runner when simulating events.

…ce utils

…phased_trigger_and_digitizer

fschlueter · 2025-01-20T16:52:21Z

@ryankrebs016 I merged the branch simulate_trigger_channels_rnog_simplified into this one and also changed the "traget" of the PR to this branch (at least for the moment). This shows better the changes you made.

fschlueter · 2025-01-20T16:56:25Z

NuRadioMC/examples/08_RNO_G_trigger_simulation/generate_data_set.py

@@ -0,0 +1,349 @@
+#!/bin/env python3


what is the purpose of this script? Compared to simulate.py does it only add the two triggers and allows to use the NuRadioMCRunner? Can we not merge the two scripts into one (maybe we can have an second script which takes the simulation as defined in the first script but uses the runners?)

fschlueter · 2025-01-20T16:59:06Z

NuRadioMC/utilities/Veff.py

-    Vrms = fin.attrs['Vrms']
+    try:
+        Vrms = fin.attrs['Vrms']
+    except:
+        Vrms = 1


hm, why is Vrms not available? It should? Is 1 a good default value? I think you might be able to make this code a one-liner Vrms = fin.attrs.get('Vrms', 1) if hdf5 attributes are compatible with python dictionaries.

fschlueter · 2025-01-20T16:59:52Z

NuRadioReco/detector/RNO_G/RNO_single_station_only_PA.json

-      "trigger_adc_noise_nbits": 3.321,
+      "trigger_adc_noise_nbits": 2.585,


has there been a change in the firmware? Is that available in the data?

No changes to firmware. See the next two comment threads for why this number changed

Okay but you removed the -1 there, why would you change this number here? Or do you only have to change the nbits if the -1 is in the voltage/adc conversion? Sorry something does not yet add up for me.

fschlueter · 2025-01-20T17:04:14Z

NuRadioReco/modules/analogToDigitalConverter.py

-
-    lsb_voltage = adc_ref_voltage / (2 ** (adc_n_bits - 1) - 1)
-
+    lsb_voltage = adc_ref_voltage / (2 ** (adc_n_bits) - 1)


See discussion in #500, if we change this (which I in principle would be very happy with) we would need to update all existing json detector files and would break the code for all users with their own detector files in a very subtle and dangerous way.

This just makes the code kind of confusing to follow. To me I would use lsb_voltage = voltage_range / (2^adc_n_bits -1). Instead, a ref_voltage is used and is calculated using the code in the comment below these.

I absolutely agree with you, I stumbled across this twice (more than a year apart from each other so I had forgotten). The issue is if we change it now we will effect all other users using this module. Maybe we should have this discussion in the issue I linked above?

fschlueter · 2025-01-20T17:04:23Z

NuRadioReco/modules/analogToDigitalConverter.py

-            adc_ref_voltage = Vrms * (2 ** (adc_n_bits - 1) - 1) / (2 ** (adc_noise_n_bits - 1) - 1)
+            adc_ref_voltage = Vrms * (2 ** (adc_n_bits) - 1) / (2 ** (adc_noise_n_bits ) - 1)


See comment above

fschlueter · 2025-01-21T09:00:05Z

I like the idea of having a base class for the phased array trigger very much! But I think we should think about restructuring the code a bit. First, can you rename the phased array trigger modules to reflect the typically name scheme for modules in NuRadio? Should we put all modules in trigger/phasedArray/?

fschlueter · 2025-02-05T15:58:32Z

NuRadioReco/modules/RNO_G/triggerBoardResponse.py

-            if np.any(mask):
-                gain_to_use = self._triggerBoardAmplifications[mask][0]
-                vrms_after_gain.append(amplified_vrms_values[mask][0])
+            volts_per_adc = self._adc_input_range / (2 ** total_bits)


there is a -1 missing (not in the exponent). If you have a 8bit system you want to divide by 2 ** 8 -1 = 255

fschlueter · 2025-02-07T18:40:33Z

NuRadioReco/modules/RNO_G/triggerBoardResponse.py


        if digitize_trace:
            self.digitize_trace(station, det, trigger_channels, ideal_vrms)
+            trigger_board_vrms = self.get_vrms(station, trigger_channels)


I think this line is a problem for two reasons. Lets start with the important one. You recalculate the vrms here which you use to define the trigger thresholds. However, if the traces are in ADC counts so should be the thresholds (I think). Hence if you self._adc_output == "counts" you have to pass the trigger_board_vrms through np.floor(trigger_board_vrms).dtype(int). The second reason, before if you passed vrms as an argument to this module, what was returned was the amplified vrms. Now you always return the std(amplified_trace). I am not sure what we acutally want to do here but point out that you change what the module does.

The first reason isn't so much an issue as the traces are gain normalized and digitized so the output is in adc or voltage. Though I agree this can be removed. It was leftover when I was calculating trace rms awkwardly and was skeptical that the gain and digitization was affecting the rms and then thresholds. The assumption would be that if thresholds are stable (on an event by event basis) then the thresholds should be based on theoretical rms values. When calculating the thresholds in simulation, the code does the gain normalization and digitization so the resulting threshold is already informed that things get digitized etc

locally I already removed this

No - when you calculate the STD from a trace of ints you do not get and int back.

When you removed this line, how do you convert the VRMS to have the "unit" ADC? I use the following code currently:

if vrms is None: vrms = self.get_noise_vrms_per_trigger_channel(station, trigger_channels) if apply_adc_gain: equalized_vrms, ideal_vrms = self.apply_adc_gain(station, det, trigger_channels, vrms) else: equalized_vrms = vrms ideal_vrms = vrms if digitize_trace: self.digitize_trace(station, det, trigger_channels, ideal_vrms) if self._adc_output == "counts": lsb_voltage = self._adc_input_range / (2 ** self._n_bits - 1) equalized_vrms = np.floor(equalized_vrms / lsb_voltage).astype(int) logger.debug("obs. Vrms {} ADC".format(equalized_vrms))

I'm confused, doesn't something like np.std(array_of_ints) return a float? Also vrms should not be forced to an int. The conversion should happen on the threshold if trying to match firmware

ryankrebs016 · 2025-02-07T19:02:47Z

Do we want to include the change from adc_noise_nbits to something like adc_noise_counts to remove the ambiguous noise calculation?

ryankrebs016 added 5 commits December 20, 2024 21:30

update phased trigger and digitizer bug fixes

697c0af

track gain to help efficiency sims and fix formatting

a7dcc79

refactor phased array, add things for eff sims, add upsampling to tra…

52c786f

…ce utils

small things

b0f77ce

add sim script to compare trigger performances

bda57df

ryankrebs016 requested a review from fschlueter January 20, 2025 16:37

Merge branch 'simulate_trigger_channels_rnog_simplified' into update_…

46979b9

…phased_trigger_and_digitizer

fschlueter changed the base branch from develop to simulate_trigger_channels_rnog_simplified January 20, 2025 16:51

fschlueter reviewed Jan 20, 2025

View reviewed changes

Minor clean up

bf059d4

fix module naming

1b147c9

fschlueter changed the base branch from simulate_trigger_channels_rnog_simplified to develop February 5, 2025 09:42

Merge branch 'develop' into update_phased_trigger_and_digitizer

0f13d05

fschlueter reviewed Feb 5, 2025

View reviewed changes

fschlueter requested changes Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update phased trigger and digitizer #799

Update phased trigger and digitizer #799

ryankrebs016 commented Jan 20, 2025

fschlueter commented Jan 20, 2025

fschlueter Jan 20, 2025

fschlueter Jan 20, 2025

fschlueter Jan 20, 2025

ryankrebs016 Jan 20, 2025

fschlueter Jan 21, 2025

fschlueter Jan 20, 2025

ryankrebs016 Jan 20, 2025 •

edited

Loading

fschlueter Jan 21, 2025

fschlueter Jan 20, 2025

fschlueter commented Jan 21, 2025

fschlueter Feb 5, 2025

fschlueter Feb 7, 2025

ryankrebs016 Feb 7, 2025

ryankrebs016 Feb 7, 2025

fschlueter Feb 7, 2025

fschlueter Feb 7, 2025

ryankrebs016 Feb 7, 2025

ryankrebs016 commented Feb 7, 2025

		"trigger_adc_noise_nbits": 3.321,
		"trigger_adc_noise_nbits": 2.585,


		lsb_voltage = adc_ref_voltage / (2 ** (adc_n_bits - 1) - 1)

		lsb_voltage = adc_ref_voltage / (2 ** (adc_n_bits) - 1)

		adc_ref_voltage = Vrms * (2 (adc_n_bits - 1) - 1) / (2 (adc_noise_n_bits - 1) - 1)
		adc_ref_voltage = Vrms * (2 (adc_n_bits) - 1) / (2 (adc_noise_n_bits ) - 1)

Update phased trigger and digitizer #799

Are you sure you want to change the base?

Update phased trigger and digitizer #799

Conversation

ryankrebs016 commented Jan 20, 2025

fschlueter commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryankrebs016 Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fschlueter commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryankrebs016 commented Feb 7, 2025

ryankrebs016 Jan 20, 2025 •

edited

Loading